-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Impose pre-determined routes instead of announcing them in the manifest #91
Conversation
latest/index.html
Outdated
@@ -638,10 +651,11 @@ <h3>Preview Metadata</h3> | |||
</section> | |||
<section> | |||
<h3>Preview Queries</h3> | |||
<p>A preview service is queried by resolving the <a>preview URL</a> for an <a>entity</a>. The URL must resolve to an HTML document. | |||
<p>A preview service is queried by resolving the URI template <code>/preview?id={id}</code> relative to the reconciliation endpoint, where <code>id</code> is subsituted by the <a>entity</a> identifier. The URL must resolve to an HTML document, | |||
which MUST be viewable in an <code>iframe</code> HTML element whose dimensions are determined by the <a>preview metadata</a>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't that be CAN instead? I mean some clients don't necessarily need an iframe, it's up to them how to interact with data coming from a standardized api spec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That MUST
does not force clients to embed the preview in an iframe, it only ensures that the server makes that possible. I would be fine with reformulating this to say something like "an HTML page which MUST be viewable in a viewport whose dimensions are…", if you want to avoid the term iframe…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes would be better to rephrase like that I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see so much text in the spec removed (+96 −246
)!
Made one small inline suggestion for a missing removal.
Approving this to keep focused on the pre-determined routes vs. announcing them, but since this removes the optional GET routes and changes the way the main reconcile
and extend
routes work, this might be the right point to reconsider if we really want to use POST here. In particular if our goal is to get closer to REST principles, I'd think that reconcile
and extend
should be GET routes, since they only query data and don't change any data on the server.
(BTW there seems to be an upcoming new verb for 'GET with a body', currently called SEARCH.)
Co-authored-by: Fabian Steeg <[email protected]>
Thanks, good catch! For GET vs POST, I guess we are still limited by the fact that:
But I get that it might also be weird to use POST in some web frameworks, typically those which enforce some sort of CSRF protection on such endpoints (which would need to be disabled for our purposes). |
I'd prefer that we stay with HTTP POST as we have defined in the latest draft. This would allow a bit more flexibility in the kinds of entities that could potentially be reconciled. For instance, I wonder if it should be optional for a service to use |
By now it's called |
Actually we want to migrate out of such content-types because they are not as REST-friendly as just |
URL is now built by resolving URI template: `/preview?id={id}`
…st (#91) * Impose pre-determined routes rather than announcing them in the manifest, for #84 * Remove mention of iframe * Remove the mention of form elements. Co-authored-by: Fabian Steeg <[email protected]> Co-authored-by: Fabian Steeg <[email protected]>
URL is now built by resolving URI template: `/preview?id={id}`
This is a first step towards #84: instead of leaving the suggest, preview and property proposal paths up to the implementer, those paths are pre-determined by the spec.
This was also the occasion to remove the flyout: I had to do this when changing the manifest structure. Sorry to be bundling that up in the same PR, I should have done another one about this earlier. Therefore it closes #42.
It also closes #32 in the same go.